[Resources.Container] Add Kubernetes support #1699

joegoldman2 · 2024-04-27T09:01:41Z

Fixes #1562.
Only tested manually at the moment.

For significant contributions please make sure you have completed the following items:

Appropriate CHANGELOG.md updated for non-trivial changes
Design discussion issue #
Changes in public API reviewed

codecov · 2024-04-27T09:02:52Z

Codecov Report

Attention: Patch coverage is 50.00000% with 44 lines in your changes missing coverage. Please review.

Project coverage is 73.04%. Comparing base (71655ce) to head (740efcb).
Report is 388 commits behind head on main.

Files	Patch %	Lines
...elemetry.Resources.Container/K8sMetadataFetcher.cs	0.00%	20 Missing ⚠️
...Telemetry.Resources.Container/ContainerDetector.cs	73.46%	13 Missing ⚠️
...esources.Container/ContainerResourceEventSource.cs	21.42%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1699      +/-   ##
==========================================
- Coverage   73.91%   73.04%   -0.88%     
==========================================
  Files         267      331      +64     
  Lines        9615    11986    +2371     
==========================================
+ Hits         7107     8755    +1648     
- Misses       2508     3231     +723

Flag	Coverage Δ
unittests-Exporter.Geneva	`53.20% <ø> (?)`
unittests-Exporter.InfluxDB	`94.65% <ø> (?)`
unittests-Exporter.Instana	`71.24% <ø> (?)`
unittests-Exporter.OneCollector	`93.07% <ø> (?)`
unittests-Exporter.Stackdriver	`75.73% <ø> (?)`
unittests-Extensions	`89.01% <ø> (?)`
unittests-Extensions.AWS	`83.41% <ø> (?)`
unittests-Extensions.Enrichment	`100.00% <ø> (?)`
unittests-Instrumentation.AWS	`90.45% <ø> (?)`
unittests-Instrumentation.AWSLambda	`87.96% <ø> (?)`
unittests-Instrumentation.AspNetCore	`85.27% <ø> (?)`
unittests-Instrumentation.ElasticsearchClient	`79.87% <ø> (?)`
unittests-Instrumentation.EntityFrameworkCore	`55.49% <ø> (?)`
unittests-Instrumentation.EventCounters	`76.36% <ø> (?)`
unittests-Instrumentation.GrpcNetClient	`79.61% <ø> (?)`
unittests-Instrumentation.Hangfire	`93.58% <ø> (?)`
unittests-Instrumentation.Http	`82.05% <ø> (?)`
unittests-Instrumentation.Process	`100.00% <ø> (?)`
unittests-Instrumentation.Quartz	`78.94% <ø> (?)`
unittests-Instrumentation.Runtime	`100.00% <ø> (?)`
unittests-Instrumentation.SqlClient	`91.89% <ø> (?)`
unittests-Instrumentation.StackExchangeRedis	`67.02% <ø> (?)`
unittests-Instrumentation.Wcf	`78.47% <ø> (?)`
unittests-PersistentStorage	`65.44% <ø> (?)`
unittests-Resources.AWS	`74.08% <100.00%> (?)`
unittests-Resources.Azure	`82.83% <ø> (?)`
unittests-Resources.Container	`57.72% <49.42%> (?)`
unittests-Resources.Gcp	`72.54% <ø> (?)`
unittests-Resources.Host	`50.00% <ø> (?)`
unittests-Resources.OperatingSystem	`87.50% <ø> (?)`
unittests-Resources.Process	`100.00% <ø> (?)`
unittests-Resources.ProcessRuntime	`94.11% <ø> (?)`
unittests-Sampler.AWS	`87.97% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/OpenTelemetry.Resources.AWS/AWSEKSDetector.cs	`57.89% <100.00%> (ø)`
...y.Resources.Container/Models/K8sContainerStatus.cs	`100.00% <100.00%> (ø)`
...OpenTelemetry.Resources.Container/Models/K8sPod.cs	`100.00% <100.00%> (ø)`
...lemetry.Resources.Container/Models/K8sPodStatus.cs	`100.00% <100.00%> (ø)`
...esources.Container/ContainerResourceEventSource.cs	`21.42% <21.42%> (ø)`
...Telemetry.Resources.Container/ContainerDetector.cs	`75.94% <73.46%> (ø)`
...elemetry.Resources.Container/K8sMetadataFetcher.cs	`0.00% <0.00%> (ø)`

... and 332 files with indirect coverage changes

src/OpenTelemetry.ResourceDetectors.Container/ContainerResourceDetector.cs

rajkumar-rangaraj · 2024-05-02T04:48:50Z

src/OpenTelemetry.ResourceDetectors.Container/ContainerResourceDetector.cs

@@ -18,6 +21,13 @@ public class ContainerResourceDetector : IResourceDetector
    private const string Filepath = "/proc/self/cgroup";
    private const string FilepathV2 = "/proc/self/mountinfo";
    private const string Hostname = "hostname";
+    private const string K8sServiceHostKey = "KUBERNETES_SERVICE_HOST";
+    private const string K8sServicePortKey = "KUBERNETES_SERVICE_PORT_HTTPS";
+    private const string K8sNamespaceKey = "KUBERNETES_NAMESPACE";


Is this environment variable and KUBERNETES_CONTAINER_NAME available in all Kubernetes environments?

From what I know, KUBERNETES_NAMESPACE and KUBERNETES_CONTAINER_NAME are not available by default in Kubernetes. The user will have to provide them using the downward API.

rajkumar-rangaraj · 2024-05-02T04:50:26Z

src/OpenTelemetry.ResourceDetectors.Container/ContainerResourceDetector.cs

+            var @namespace = Environment.GetEnvironmentVariable(K8sNamespaceKey);
+            var hostname = Environment.GetEnvironmentVariable(K8sHostnameKey);
+            var containerName = Environment.GetEnvironmentVariable(K8sContainerNameKey);
+            var url = $"https://{host}:{port}/api/v1/namespaces/{@namespace}/pods/{hostname}";


What is the guarantee that this will be a well-formed URL?

Nothing I think but in this case an exception will be thrown, catched and logged using the EventSource.

iskiselev · 2024-05-03T00:55:37Z

src/OpenTelemetry.ResourceDetectors.Container/ContainerResourceDetector.cs

+    private const string K8sServicePortKey = "KUBERNETES_SERVICE_PORT_HTTPS";
+    private const string K8sNamespaceKey = "KUBERNETES_NAMESPACE";
+    private const string K8sHostnameKey = "HOSTNAME";
+    private const string K8sContainerNameKey = "KUBERNETES_CONTAINER_NAME";


The tricky part here - k8s doesn't have any standard variable to provide container name. It is entirely based on the user. I don't see any issues that extractor would expect it to be named "KUBERNETES_CONTAINER_NAME" - but probably it may be better to be passed by user to detector as constructor argument.
If it is passed by user, we can also report it as additional attribute.

I think it's the same for KUBERNETES_NAMESPACE in this case, which is not standard as well. Perhaps we can let the user supply the values via the constructor but also keep automatic detection via the environment variables if they are not supplied. What do you think?

If it is passed by user, we can also report it as additional attribute.

I'm not sure that's the purpose of this detector. From my point of view, the user can already use the env variable OTEL_RESOURCE_ATTRIBUTES or implement his own detector to add this resource attribute.

github-actions · 2024-05-15T03:17:15Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

iskiselev · 2024-05-15T03:29:31Z

Not stale. I'll try to finish review this week.

github-actions · 2024-07-06T03:17:03Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

joegoldman2 · 2024-07-06T09:23:19Z

Not stale. Waiting for @iskiselev's feedback.

github-actions · 2024-07-14T03:17:04Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

iskiselev · 2024-07-14T04:02:16Z

Not stale. open-telemetry/opentelemetry-specification#4140 opened to discuss next step.
@joegoldman2, if new config environment variables will be approved, we will still need to make it configurable though factory method parameters per:
https://github.com/open-telemetry/opentelemetry-specification/blob/27bc9ffad2b386626367dd2f98d0ba2ddf35a18f/specification/configuration/sdk-environment-variables.md?plain=1#L54

joegoldman2 · 2024-07-15T16:50:36Z

Ok, so I guess the next step is to define the desired API to allow users to set the values programmatically, right?

iskiselev · 2024-07-15T20:50:19Z

@joegoldman2, I already suggested API at #1699 (comment), but any would works once it allows to specify values.
I'm not sure, whether we will see any move open-telemetry/opentelemetry-specification#4140. With program API we should be able to merge this PR even without environment values support if consensus would not be reached.
Btw, do you see an option to merge it already (with @Kielek approval) or is it requires anything else? I still fully agree with @Kielek , that we should either get common agreement on environment values usage and document them - or do not use them at all.

iskiselev · 2024-07-15T20:58:01Z

And looks like new environment variable would not be added any time soon: open-telemetry/opentelemetry-specification#2891 (comment). So, using API-based values may be the only option.

joegoldman2 · 2024-07-17T10:55:33Z

Thank you for the API proposal #1699 (comment). What is the procedure to review/validate a new API?

iskiselev · 2024-07-27T01:13:43Z

@joegoldman2, I actually don't know what should be the procedure to review/validate a new API. From my point of view (I'm curently codeowner for src/OpenTelemetry.Resources.Container), anything that will give a way to provide required values from code should work, as it will be in compliance with specification. As it will be isolated as arguments of AddContainerDetector method, we probably don't need any wider audience review. Alternatvely we can try to visit Tuesday meeting and seek opinions / advices there.

iskiselev · 2024-07-27T01:16:53Z

BTW, we just finished validation of using k8sattributes processor on collector, and it perfectly solves the problem of resovling container.id for k8s cluster (once k8s.container.name attibute added).

github-actions · 2024-08-03T03:16:33Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

src/OpenTelemetry.Resources.Container/ContainerResourceEventSource.cs

…urce.cs Co-authored-by: Weihan Li <weihanli@outlook.com>

joegoldman2 · 2024-08-06T11:27:59Z

BTW, we just finished validation of using k8sattributes processor on collector, and it perfectly solves the problem of resovling container.id for k8s cluster (once k8s.container.name attibute added).

Do you have a link to the related issue?

Add Kubernetes support in Container Resource Detector

df0e7a4

joegoldman2 requested a review from a team as a code owner April 27, 2024 09:01

joegoldman2 had a problem deploying to external April 27, 2024 09:01 — with GitHub Actions Failure

github-actions bot assigned iskiselev, ppittle and srprash Apr 27, 2024

Update CHANGELOG.md

7a966f3

joegoldman2 had a problem deploying to external April 27, 2024 09:02 — with GitHub Actions Failure

Fix warnings

c7a08b0

joegoldman2 had a problem deploying to external April 27, 2024 09:12 — with GitHub Actions Failure

Return string.Empty if container id is null or empty

0beaa51

joegoldman2 had a problem deploying to external April 27, 2024 09:22 — with GitHub Actions Failure

joegoldman2 changed the title ~~Add Kubernetes support in Container Resource Detector~~ [ResourceDetectors.Container] Add Kubernetes support Apr 27, 2024

Update CHANGELOG.md

966b860

joegoldman2 had a problem deploying to external April 27, 2024 12:03 — with GitHub Actions Failure

Remove blank line

fde4504

joegoldman2 had a problem deploying to external April 27, 2024 12:06 — with GitHub Actions Failure

matt-hensley reviewed Apr 30, 2024

View reviewed changes

src/OpenTelemetry.ResourceDetectors.Container/ContainerResourceDetector.cs Outdated Show resolved Hide resolved

src/OpenTelemetry.ResourceDetectors.Container/ContainerResourceDetector.cs Outdated Show resolved Hide resolved

Apply suggestions from review

2af92bc

joegoldman2 had a problem deploying to external April 30, 2024 19:10 — with GitHub Actions Failure

rajkumar-rangaraj reviewed May 2, 2024

View reviewed changes

iskiselev reviewed May 3, 2024

View reviewed changes

Kielek added the comp:resources.container Things related to OpenTelemetry.Resources.Container label May 6, 2024

Merge branch 'main' into fix/1562

1a3b5e6

joegoldman2 had a problem deploying to external May 7, 2024 06:02 — with GitHub Actions Failure

github-actions bot added the Stale label May 15, 2024

github-actions bot added the Stale label Jul 6, 2024

github-actions bot removed the Stale label Jul 7, 2024

iskiselev mentioned this pull request Jul 11, 2024

Environment variables to provide K8s container and pod name open-telemetry/opentelemetry-specification#4140

Open

github-actions bot added the Stale label Jul 14, 2024

github-actions bot removed the Stale label Jul 15, 2024

Merge branch 'main' into fix/1562

5a4c937

joegoldman2 had a problem deploying to external July 23, 2024 07:15 — with GitHub Actions Failure

github-actions bot added the Stale label Aug 3, 2024

WeihanLi reviewed Aug 5, 2024

View reviewed changes

src/OpenTelemetry.Resources.Container/ContainerResourceEventSource.cs Outdated Show resolved Hide resolved

github-actions bot removed the Stale label Aug 6, 2024

Update src/OpenTelemetry.Resources.Container/ContainerResourceEventSo…

fed4283

…urce.cs Co-authored-by: Weihan Li <weihanli@outlook.com>

joegoldman2 requested a deployment to external August 6, 2024 11:27 — with GitHub Actions Waiting

Merge branch 'main' into fix/1562

1e9ecbf

joegoldman2 requested a deployment to external August 6, 2024 11:28 — with GitHub Actions Waiting

Merge branch 'main' into fix/1562

2ba517e

joegoldman2 requested a deployment to external August 6, 2024 16:10 — with GitHub Actions Waiting

Merge branch 'main' into fix/1562

740efcb

joegoldman2 requested a deployment to external August 17, 2024 11:45 — with GitHub Actions Waiting

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Resources.Container] Add Kubernetes support #1699

[Resources.Container] Add Kubernetes support #1699

joegoldman2 commented Apr 27, 2024

codecov bot commented Apr 27, 2024 •

edited

Loading

rajkumar-rangaraj May 2, 2024

joegoldman2 May 2, 2024

rajkumar-rangaraj May 2, 2024

joegoldman2 May 2, 2024

iskiselev May 3, 2024

joegoldman2 May 3, 2024

github-actions bot commented May 15, 2024

iskiselev commented May 15, 2024

github-actions bot commented Jul 6, 2024

joegoldman2 commented Jul 6, 2024

github-actions bot commented Jul 14, 2024

iskiselev commented Jul 14, 2024

joegoldman2 commented Jul 15, 2024

iskiselev commented Jul 15, 2024

iskiselev commented Jul 15, 2024

joegoldman2 commented Jul 17, 2024

iskiselev commented Jul 27, 2024 •

edited

Loading

iskiselev commented Jul 27, 2024

github-actions bot commented Aug 3, 2024

joegoldman2 commented Aug 6, 2024

[Resources.Container] Add Kubernetes support #1699

Are you sure you want to change the base?

[Resources.Container] Add Kubernetes support #1699

Conversation

joegoldman2 commented Apr 27, 2024

codecov bot commented Apr 27, 2024 • edited Loading

Codecov Report

rajkumar-rangaraj May 2, 2024

Choose a reason for hiding this comment

joegoldman2 May 2, 2024

Choose a reason for hiding this comment

rajkumar-rangaraj May 2, 2024

Choose a reason for hiding this comment

joegoldman2 May 2, 2024

Choose a reason for hiding this comment

iskiselev May 3, 2024

Choose a reason for hiding this comment

joegoldman2 May 3, 2024

Choose a reason for hiding this comment

github-actions bot commented May 15, 2024

iskiselev commented May 15, 2024

github-actions bot commented Jul 6, 2024

joegoldman2 commented Jul 6, 2024

github-actions bot commented Jul 14, 2024

iskiselev commented Jul 14, 2024

joegoldman2 commented Jul 15, 2024

iskiselev commented Jul 15, 2024

iskiselev commented Jul 15, 2024

joegoldman2 commented Jul 17, 2024

iskiselev commented Jul 27, 2024 • edited Loading

iskiselev commented Jul 27, 2024

github-actions bot commented Aug 3, 2024

joegoldman2 commented Aug 6, 2024

codecov bot commented Apr 27, 2024 •

edited

Loading

iskiselev commented Jul 27, 2024 •

edited

Loading